Support sessionFs in Node SDK. Update runtime.#917
Conversation
5b7cfff to
152e970
Compare
Cross-SDK Consistency ReviewI've reviewed this PR for consistency with the other SDK implementations (Python, Go, and .NET). This PR introduces a custom session data store feature to the Node.js/TypeScript SDK only. Summary of ChangesThis PR adds:
Consistency Gap IdentifiedFinding: This feature is not yet implemented in Python, Go, or .NET SDKs. The equivalent configuration options in other SDKs are:
None of these currently have a RecommendationSince this PR is marked as Draft and the body says "Not yet ready," I suggest:
ConclusionNo immediate action required since this is a draft PR. However, before marking this feature as generally available, consider implementing equivalent functionality in Python, Go, and .NET to maintain feature parity across the SDK suite. If this is intended as a Node.js-exclusive feature, that should be clearly documented with rationale. Great work on the test coverage and RPC handler design! The approach is clean and extensible. 👍
|
Cross-SDK Consistency ReviewI've reviewed this PR for consistency across the four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). SummaryThis PR introduces a custom session data store feature to the Node.js/TypeScript SDK only. This creates a feature parity gap with the other SDK implementations. What This PR AddsThe PR adds a
Cross-SDK Status
RecommendationTo maintain feature parity, this capability should be added to the other three SDKs. Here's how it would map to each language's conventions: Python (
|
152e970 to
3d11cff
Compare
Cross-SDK Consistency ReviewThis PR introduces a significant new feature: custom session data storage backend support via the What This PR Adds (Node.js/TypeScript only)
Missing From Other SDKsPython (
|
Cross-SDK Consistency Review: Session Filesystem ProviderThis PR introduces a new session filesystem provider feature to the Node.js/TypeScript SDK only. This represents a consistency gap across the SDK implementations. Feature SummaryThe PR adds the ability to provide a custom storage backend for session state via the
Added APIs:
Cross-SDK Consistency AnalysisCurrent state:
RecommendationThis is a useful extensibility feature that would benefit all SDK implementations. Consider whether:
Implementation Notes (if extending to other SDKs)The feature requires:
Python equivalent naming: class SessionFsHandler(Protocol):
async def read_file(self, params: SessionFsReadFileParams) -> SessionFsReadFileResult: ...
async def write_file(self, params: SessionFsWriteFileParams) -> None: ...
# ... etcGo equivalent naming: type SessionFsHandler interface {
ReadFile(ctx context.Context, params SessionFsReadFileParams) (SessionFsReadFileResult, error)
WriteFile(ctx context.Context, params SessionFsWriteFileParams) error
// ... etc
}.NET equivalent naming: public interface ISessionFsHandler {
Task(SessionFsReadFileResult) ReadFileAsync(SessionFsReadFileParams params, CancellationToken cancellationToken = default);
Task WriteFileAsync(SessionFsWriteFileParams params, CancellationToken cancellationToken = default);
// ... etc
}Status: This PR is architecturally sound for the Node.js SDK. The consistency gap is flagged for awareness and future planning, not as a blocker for this PR.
|
|
I am very interested in this feature. We would love to use something like redis to store the session data so that I can use this in a multi instance deployment. Providing a virtual file system abstraction instead of session data storage abstraction (as originally coded) is a bit strange to me. It is awkward to map to something like redis or mongo which are not structured like filesystems. In the commit, copilot-agent-runtime#5432 is mentioned, but there is no link, could you provide a link to that discussion? is that public? |
Cross-SDK Consistency ReviewThis PR continues to add session filesystem provider support exclusively to the Node.js/TypeScript SDK. Previous reviews have thoroughly documented this consistency gap (see March 27 review). The feature remains unimplemented in Python, Go, and .NET. Latest Changes (March 30)The most recent commit ("Expand API surface slightly") made minor additions to the generated RPC types and TypeScript types—no cross-SDK implications. Key Observations
RecommendationGiven external user interest and the value for distributed/cloud scenarios, consider the rollout strategy:
If choosing Option A, document the feature gap in other SDK READMEs and create tracking issues for Python/Go/.NET implementations. No new consistency issues introduced since the last review. The core consistency gap (Node.js-only feature) remains.
|
Cross-SDK Consistency Review: Session Filesystem ProviderThis PR introduces a custom session filesystem provider feature that allows Node.js SDK users to provide their own storage implementation for session-scoped files, routing all file I/O through custom callbacks instead of the server's default local filesystem. ✅ Current Implementation (Node.js only)The PR adds:
|
Cross-SDK Consistency ReviewThis PR adds support for custom session filesystem providers to the Node.js/TypeScript SDK only, creating a feature gap across the multi-language SDK implementations. What's Added (Node.js SDK)The PR introduces:
Feature Parity Status
RecommendationThis appears to be an infrastructure feature that enables advanced use cases like:
Should this feature be available in all SDKs? Consider:
Since this is marked as a draft PR, this may be an experimental feature. If it moves toward merge, I recommend creating tracking issues for the other SDKs to maintain consistency. Note: No inline comments added since this is a new feature addition (not modifying existing code). The implementation looks well-structured with proper RPC handler registration via
|
Cross-SDK Consistency ReviewThis PR adds a significant new feature to the Node.js/TypeScript SDK only: Custom Session Filesystem Provider. What's Added (Node.js SDK)This PR introduces the ability for applications to provide their own filesystem implementation for session-scoped storage:
Impact on Other SDKsStatus: ❌ This feature does NOT exist in Python, Go, or .NET SDKs This creates a feature parity gap across the SDK implementations. Applications using other language SDKs cannot:
RecommendationConsider whether this feature should be added to the other SDKs to maintain cross-language consistency. This is particularly important because:
Suggested Path ForwardIf this feature should be available across all SDKs (which I recommend given its architectural significance):
The RPC types are likely already generated in other languages from the same schema, so the main work would be wiring up the client-side registration and handler interfaces. Note: This is marked as a Draft PR, so this may already be on the roadmap. If cross-SDK support is planned, consider implementing it before finalizing to avoid releasing a Node-only API that creates expectations.
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #917
| * on connection, routing all session-scoped file I/O through these callbacks | ||
| * instead of the server's default local filesystem storage. | ||
| */ | ||
| sessionFs?: SessionFsConfig; |
There was a problem hiding this comment.
Cross-SDK Consistency: This new client-level option does not exist in Python, Go, or .NET SDKs.
Equivalent locations in other SDKs:
- Python: Would need to be added to
SubprocessConfig/ExternalServerConfiginpython/copilot/client.py - Go: Would need to be added to
ClientOptionsstruct ingo/types.go - .NET: Would need to be added to
CopilotClientOptionsclass indotnet/src/Types.cs
| * Supplies a handler for session filesystem operations. This takes effect | ||
| * only if {@link CopilotClientOptions.sessionFs} is configured. | ||
| */ | ||
| createSessionFsHandler?: (session: CopilotSession) => SessionFsHandler; |
There was a problem hiding this comment.
Cross-SDK Consistency: This session-level callback does not exist in Python, Go, or .NET SDKs.
Equivalent locations in other SDKs:
- Python: Would need to be added to the
create_session()method parameters inpython/copilot/client.py - Go: Would need to be added to
SessionConfigstruct ingo/types.go - .NET: Would need to be added to
SessionConfigclass indotnet/src/Types.cs
The handler interface (SessionFsHandler) would need similar language-specific definitions.
| @@ -399,6 +404,15 @@ export class CopilotClient { | |||
| // Verify protocol version compatibility | |||
There was a problem hiding this comment.
Cross-SDK Consistency: This registration logic for the session filesystem provider does not exist in other SDKs.
Equivalent locations:
- Python: Would need RPC call in
_connect()method inpython/copilot/client.py - Go: Would need RPC call in
Connect()method ingo/client.go - .NET: Would need RPC call in
ConnectAsync()method indotnet/src/Client.cs
| @@ -1562,6 +1596,14 @@ export class CopilotClient { | |||
| await this.handleSystemMessageTransform(params) | |||
There was a problem hiding this comment.
Cross-SDK Consistency: This handler registration pattern (registerClientSessionApiHandlers) does not exist in other SDKs.
Equivalent locations:
- Python: Would need similar RPC handler registration in
_connect()inpython/copilot/client.py - Go: Would need similar handler registration in
Connect()ingo/client.go - .NET: Would need similar handler registration in
ConnectAsync()indotnet/src/Client.cs
The generated RPC files (python/copilot/generated/rpc.py, go/rpc/rpc.go, dotnet/src/Generated/Rpc.cs) likely already have the type definitions for these methods if they're code-generated from the same schema.
| } | ||
|
|
||
| export interface McpConfigListResult { | ||
| /** |
There was a problem hiding this comment.
Cross-SDK Consistency: The SessionFsHandler interface and related types (like SessionFsReadFileParams, etc.) are currently only in the Node.js generated RPC file.
Code generation impact: When this feature is added to other SDKs, the RPC schema will need to be updated first, which will then generate the corresponding types in:
python/copilot/generated/rpc.py(Python)go/rpc/generated_rpc.go(Go)dotnet/src/Generated/Rpc.cs(.NET)
Currently, searching these files shows no sessionFs/SessionFs types, confirming they haven't been generated yet for other languages.
- Add sessionDataStore option to CopilotClientOptions - Extend codegen to generate client API handler types (SessionDataStoreHandler) - Register as session data storage provider on connection via sessionDataStore.setDataStore RPC - Add E2E tests for persist, resume, list, delete, and reject scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Migrate the TypeScript SDK from the event-level sessionDataStore abstraction to the general-purpose SessionFs virtual filesystem, matching the runtime's new design (copilot-agent-runtime#5432). Key changes: - Regenerate RPC types from runtime schema with sessionFs.* methods - Replace SessionDataStoreConfig with SessionFsConfig (initialCwd, sessionStatePath, conventions + 9 filesystem handler callbacks) - Client calls sessionFs.setProvider on connect (was setDataStore) - Client registers sessionFs.* RPC handlers (readFile, writeFile, appendFile, exists, stat, mkdir, readdir, rm, rename) - New E2E tests with InMemorySessionFs (filesystem-level, not events) - Remove old session_store tests and snapshots
a482499 to
45b44c7
Compare
e534ed5 to
a938ece
Compare
Cross-SDK Consistency Review: sessionFs FeatureThank you for implementing the SummaryThis PR adds support for virtualizing per-session storage (event logs, large output handling, etc.) through a custom filesystem provider. The implementation is currently Node.js/TypeScript only, as noted in the PR description: "Currently, client support is in the Node SDK only but will be added to others soon." What Was Added (Node.js SDK)Client-level configuration:
Session-level integration:
Generated types (all SDKs):
Testing:
Current State: Feature Parity Gap
Recommendations✅ This PR is fine to merge since:
📋 For follow-up work, the other SDKs will need: Python SDK (
Go SDK (
.NET SDK (
API Design Consistency NoteThe Node.js implementation follows excellent patterns that should be preserved when implementing in other languages:
When implementing in other SDKs, please maintain these design principles while adapting to language idioms:
Conclusion: ✅ No blocking issues for this PR. The generated types ensure the other SDKs can implement this feature consistently when ready. Consider opening follow-up issues to track implementation in Python, Go, and .NET to maintain feature parity.
|
There was a problem hiding this comment.
Pull request overview
Adds Node.js SDK support for session-scoped filesystem virtualization (“sessionFs”) so the runtime can route per-session storage (event log, large tool outputs, compaction artifacts) through client-provided callbacks, alongside a runtime/schema update and new E2E coverage.
Changes:
- Add
sessionFsclient option + per-sessioncreateSessionFsHandlerwiring and JSON-RPC handler registration in the Node SDK. - Update codegen to support nullable/void RPC results and generate client-session RPC handler registration (TS), plus regenerate RPC/session-event artifacts across languages.
- Add Node E2E tests + replay snapshots; improve snapshot normalization for large-output temp file paths.
Reviewed changes
Copilot reviewed 19 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/session_lifecycle/should_support_multiple_concurrent_sessions.yaml | Updates concurrency lifecycle snapshot ordering/content. |
| test/snapshots/session_fs/should_succeed_with_compaction_while_using_sessionfs.yaml | New replay snapshot for compaction while using sessionFs. |
| test/snapshots/session_fs/should_route_file_operations_through_the_session_fs_provider.yaml | New replay snapshot validating file I/O goes through sessionFs. |
| test/snapshots/session_fs/should_reject_setprovider_when_sessions_already_exist.yaml | New replay snapshot for provider-set rejection scenario. |
| test/snapshots/session_fs/should_map_large_output_handling_into_sessionfs.yaml | New replay snapshot for large tool output temp-file behavior. |
| test/snapshots/session_fs/should_load_session_data_from_fs_provider_on_resume.yaml | New replay snapshot validating resume loads from provider. |
| test/harness/replayingCapiProxy.ts | Adds default tool-result normalization for large-output temp filenames and wildcard normalizers. |
| scripts/codegen/utils.ts | Extends API schema typing (adds clientSession, makes result nullable). |
| scripts/codegen/typescript.ts | Generates void results, includes clientSession RPCs, and emits handler registration helpers. |
| scripts/codegen/csharp.ts | Adjusts enum type resolution in generated method signatures. |
| python/copilot/generated/session_events.py | Regenerates events (adds aborted, adds needs-auth status). |
| python/copilot/generated/rpc.py | Regenerates RPC types (adds MCP config + sessionFs setProvider types, status updates). |
| nodejs/test/e2e/session_fs.test.ts | New E2E suite validating sessionFs routing, resume behavior, large output, and compaction. |
| nodejs/test/e2e/harness/sdkTestContext.ts | Allows passing through CopilotClientOptions into the E2E harness. |
| nodejs/src/types.ts | Adds sessionFs option and createSessionFsHandler session config typing + exports. |
| nodejs/src/session.ts | Adds clientSessionApis container for per-session client RPC handlers. |
| nodejs/src/index.ts | Re-exports SessionFsConfig and SessionFsHandler. |
| nodejs/src/generated/session-events.ts | Regenerated TS events (adds aborted, adds needs-auth to statuses). |
| nodejs/src/generated/rpc.ts | Regenerated RPC types + server mcp/sessionFs APIs + client-session handler registration. |
| nodejs/src/client.ts | Registers provider on connect; wires per-session handler into dispatch; registers client-session RPC handlers. |
| nodejs/package.json | Bumps @github/copilot runtime and adds @platformatic/vfs devDependency for tests. |
| nodejs/package-lock.json | Lockfile updates for runtime bump and new dev dependency. |
| go/rpc/generated_rpc.go | Regenerated Go RPC types (adds MCP/sessionFs + status updates). |
| go/generated_session_events.go | Regenerated Go events (adds aborted, adds needs-auth). |
| dotnet/src/Generated/SessionEvents.cs | Regenerated .NET events (adds aborted, adds needs-auth). |
| dotnet/src/Generated/Rpc.cs | Regenerated .NET RPC types (adds sessionFs types, adds MCP placeholder API). |
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
nodejs/src/client.ts:825
- Same leak as createSession: if sessionFs is enabled and
createSessionFsHandleris missing, this throws after adding the session tothis.sessions. Either validate before registering the session or remove it from the map before throwing.
this.sessions.set(sessionId, session);
if (this.sessionFsConfig) {
if (config.createSessionFsHandler) {
session.clientSessionApis.sessionFs = config.createSessionFsHandler(session);
} else {
throw new Error(
"createSessionFsHandler is required in session config when sessionFs is enabled in client options."
);
}
| this.sessions.set(sessionId, session); | ||
| if (this.sessionFsConfig) { | ||
| if (config.createSessionFsHandler) { | ||
| session.clientSessionApis.sessionFs = config.createSessionFsHandler(session); | ||
| } else { | ||
| throw new Error( | ||
| "createSessionFsHandler is required in session config when sessionFs is enabled in client options." | ||
| ); | ||
| } |
There was a problem hiding this comment.
When sessionFs is enabled and createSessionFsHandler is missing, this throws after the session has already been added to this.sessions, leaving a leaked/half-initialized session in the map. Validate createSessionFsHandler before this.sessions.set(...), or ensure you this.sessions.delete(sessionId) before throwing so the client state stays consistent.
This issue also appears on line 817 of the same file.
| this.sessions.set(sessionId, session); | |
| if (this.sessionFsConfig) { | |
| if (config.createSessionFsHandler) { | |
| session.clientSessionApis.sessionFs = config.createSessionFsHandler(session); | |
| } else { | |
| throw new Error( | |
| "createSessionFsHandler is required in session config when sessionFs is enabled in client options." | |
| ); | |
| } | |
| if (this.sessionFsConfig && !config.createSessionFsHandler) { | |
| throw new Error( | |
| "createSessionFsHandler is required in session config when sessionFs is enabled in client options." | |
| ); | |
| } | |
| this.sessions.set(sessionId, session); | |
| if (this.sessionFsConfig && config.createSessionFsHandler) { | |
| session.clientSessionApis.sessionFs = config.createSessionFsHandler(session); |
| "zod": "^4.3.6" | ||
| }, | ||
| "devDependencies": { | ||
| "@platformatic/vfs": "^0.3.0", |
There was a problem hiding this comment.
@platformatic/vfs@^0.3.0 declares engines: { node: ">= 22" } (visible in the updated package-lock), but this package’s engines.node is >=20.0.0. Adding a devDependency that requires Node 22 can make npm install / tests fail for contributors on Node 20/21. Consider using a vfs library compatible with Node >=20, pinning to a compatible version, or replacing it with a small in-test in-memory fs implementation.
| "@platformatic/vfs": "^0.3.0", | |
| "@platformatic/vfs": "0.2.0", |
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { SessionCompactionCompleteEvent } from "@github/copilot/sdk"; |
There was a problem hiding this comment.
This import is only used as a TypeScript type annotation. Use a type-only import (import type { ... }) to avoid pulling @github/copilot/sdk into the runtime module graph during tests and to keep emitted JS clean.
| import { SessionCompactionCompleteEvent } from "@github/copilot/sdk"; | |
| import type { SessionCompactionCompleteEvent } from "@github/copilot/sdk"; |
| useStdio: false, // Use TCP so we can connect from a second client | ||
| env, | ||
| }); | ||
| await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); |
There was a problem hiding this comment.
The should reject setProvider... test creates a new CopilotClient (useStdio: false) but never stops it, and it also creates a session without disconnecting it. This can leak a CLI process/socket across tests and cause flakiness. Add onTestFinished(() => client.forceStop()) (or await client.stop() in a finally) and ensure the created session is disconnected.
| await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); | |
| onTestFinished(() => client.forceStop()); | |
| const session = await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); | |
| onTestFinished(async () => { | |
| await session.disconnect(); | |
| }); |
| const client = new CopilotClient({ | ||
| useStdio: false, // Use TCP so we can connect from a second client | ||
| env, | ||
| }); | ||
| await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); | ||
|
|
||
| // Get the port the first client's runtime is listening on | ||
| const port = (client as unknown as { actualPort: number }).actualPort; | ||
|
|
||
| // Second client tries to connect with a session fs — should fail | ||
| // because sessions already exist on the runtime. | ||
| const client2 = new CopilotClient({ | ||
| env, | ||
| logLevel: "error", | ||
| cliUrl: `localhost:${port}`, |
There was a problem hiding this comment.
This test reaches into CopilotClient’s private actualPort via a cast. That’s brittle (renames/refactors will silently break the test). Prefer exposing a small public accessor on CopilotClient for the bound address/port (even if marked @internal), or configure a known port for the runtime in the test so it doesn’t need private state.
| const client = new CopilotClient({ | |
| useStdio: false, // Use TCP so we can connect from a second client | |
| env, | |
| }); | |
| await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); | |
| // Get the port the first client's runtime is listening on | |
| const port = (client as unknown as { actualPort: number }).actualPort; | |
| // Second client tries to connect with a session fs — should fail | |
| // because sessions already exist on the runtime. | |
| const client2 = new CopilotClient({ | |
| env, | |
| logLevel: "error", | |
| cliUrl: `localhost:${port}`, | |
| // Use a shared CLI URL so both clients talk to the same runtime without | |
| // reaching into CopilotClient's private state. | |
| const cliUrl = env.COPILOT_TEST_CLI_URL ?? env.COPILOT_CLI_URL; | |
| const client = new CopilotClient({ | |
| env, | |
| logLevel: "error", | |
| cliUrl, | |
| }); | |
| await client.createSession({ onPermissionRequest: approveAll, createSessionFsHandler }); | |
| // Second client tries to connect with a session fs — should fail | |
| // because sessions already exist on the runtime. | |
| const client2 = new CopilotClient({ | |
| env, | |
| logLevel: "error", | |
| cliUrl, |
| ); | ||
| return { entries }; | ||
| }, | ||
| rm: async ({ path }) => { |
There was a problem hiding this comment.
rm ignores the RPC params recursive and force and always calls unlink. If the runtime ever requests deleting a directory (or uses recursive: true), this handler will fail. Either implement directory removal semantics (including recursive + force) against the provider, or explicitly assert/throw with a clear error when unsupported so failures are easier to diagnose.
| rm: async ({ path }) => { | |
| rm: async ({ path, recursive, force }) => { | |
| if (recursive || force) { | |
| throw new Error( | |
| "SessionFs rm: 'recursive' and 'force' options are not supported by the test VirtualProvider handler." | |
| ); | |
| } |
| export interface RpcMethod { | ||
| rpcMethod: string; | ||
| params: JSONSchema7 | null; | ||
| result: JSONSchema7; | ||
| result: JSONSchema7 | null; | ||
| stability?: string; | ||
| } |
There was a problem hiding this comment.
RpcMethod.result is now nullable, but not all generators handle null yet. For example scripts/codegen/csharp.ts still passes method.result into emitRpcClass(...) which expects a non-null schema and will now fail typechecking (and/or at runtime) for void-returning RPCs (e.g. methods that generate Promise<void> in TS). Update the other generators to guard on method.result and emit a void/no-result method shape accordingly.
| internal ServerMcpApi(JsonRpc rpc) | ||
| { | ||
| _rpc = rpc; | ||
| } |
There was a problem hiding this comment.
ServerMcpApi is generated and exposed on ServerRpc, but it contains no methods. Given the TS SDK generates nested mcp.config.* RPCs, this looks like the C# RPC generator doesn’t recurse into nested server groups (e.g. mcp -> config -> list/add/...). Either update the generator to emit nested group APIs, or avoid emitting empty placeholder APIs to prevent a misleading surface area.
| } | |
| } | |
| /// <summary> | |
| /// MCP server APIs are not currently supported by this .NET SDK. | |
| /// This method will always throw <see cref="NotSupportedException"/>. | |
| /// </summary> | |
| /// <param name="cancellationToken">Cancellation token (currently unused).</param> | |
| /// <returns>A task that always faults with <see cref="NotSupportedException"/>.</returns> | |
| public Task EnsureSupportedAsync(CancellationToken cancellationToken = default) | |
| { | |
| return Task.FromException(new NotSupportedException("MCP server APIs are not currently supported by the GitHub Copilot .NET SDK.")); | |
| } |
ff25cc2 to
985543a
Compare
Cross-SDK Consistency Review: sessionFs FeatureSummaryThis PR introduces Findings✅ Node.js SDK (Complete Implementation)
|
Adds support for virtualizing the per-session storage that the underlying runtime uses. This includes the event log and large output handling. It doesn't yet include other workspace files.
Currently, client support is in the Node SDK only but will be added to others soon.